If the uploader gets the version wrong, couldn't this corrupt the release? There doesn't appear to be any validation of this new_key. I tested, and if there are artifacts then you get a 500 error anyway, but it breaks the whole import. If there are no artifacts, it corrupts silently. I assume that this will cause problems elsewhere, but I didn't check (I mean, release key is proj-0.1 but version is 0.2).

Quite right, and that hits something that bugged me when I did this - I've changed to start deriving the release keys instead of exposing them. For the scope of this page, it doesn't really matter that you can't change a key, you create a new one and reassign the artifacts to it - you can always remove the empty release as a separate operation by merging it with another.

There was another bug here - if you change version and nothing else your edit was silently dropped.

So this is looking up artifacts by (download path, filename)? Problem is, since download path is an exported field, it's also editable. If anybody changes it, how will it know what's being moved? So, any download path edit becomes a crash: it's an actual 500.

The path is the identity, so we should be using it to look up the existing one, and it's the only real conflict in seeding/correcting artifacts (you can't use the same full artifact path in more than one release). The add path now checks to make sure there's no conflict there, but I'm considering actually having the uploaded artifact CSV be an exhaustive list - remove all that don't match from the same PMC. This is for correcting the seeds, after all.

I guess if target_release_key in snapshot.release_keys was intended to be the global check here (it's below in the code), but because there's a continue it's never reached. Basically this check is only looking at whether it's a release key in the existing committee, whereas the later check, which is not reached, would check whether it's a release key in the whole database. Small point, but also easy to fix.

Good spot. It's the two separate branches - the latter being one for moving to another committee. So we don't actually check existence when we're adding a new row! That second check is still correct for moving a release, but we do need to check for conflict in the first case.

This contains sql.Task.version_key, but it shouldn't do because that's not a release key (example-0.1), it's just the version (0.1):

RELEASE_KEY_REFS: Final[list[tuple[type[sqlmodel.SQLModel], str]]] = [
    (sql.Release, "key"),
    (sql.Artifact, "release_key"),
    (sql.LifecycleEvent, "version_key"),
    (sql.CheckResult, "release_key"),
    (sql.BallotPaper, "release_key"),
    (sql.Distribution, "release_key"),
    (sql.Quarantined, "release_key"),
    (sql.Revision, "release_key"),
    (sql.Task, "version_key"),
]

So I think Tasks will have to be handled separately.

The (sql.LifecycleEvent, "version_key") entry looks suspicious too, but I didn't check it.

Two issues here - 1. Claude was overzealous in finding release keys and I missed it (it identified version_key there because it is called that in LifecycleEvent - I'll raise a bug and fix later). 2. I neglected to note in this part that tasks don't need moved since the whole admin page doesn't let you edit projects that have already started using ATR properly (since mangling releases, artifacts etc including on-disk and draft etc. just seems fraught with peril). This is purely for catalogued projects/releases/artifacts which exist in SVN but have no ATR-managed files anywhere. Which won't have tasks!

This only seems to check the destination, not the source.

Below, if current_project in snapshot.managed_project_keys checks the source for releases. Artifact sources aren't checked.

This is a check actually intended to prevent using these tools to manipulate ATR-managed releases (where we'll have on-disk files, revisions, etc). But we should be checking the source as well.

This field is default=True, but because browsers don't send anything when the field is unchecked, that's when we apply the default. So when the field is checked, the browser sends "on" and we set True. If the field is unchecked, the browser sends nothing, and we use the default, which is True. Therefore it's impossible to turn this field off.

I wonder if we can add a static lint for this. Might be difficult because the field element is a consequence of the Python type of the property.

Ah, valid. Yes we should consider a lint for it as a possibility. I've reversed the logic so it's now checked == overwrite

So seen_dists is what was seen in the file, not in the db? But ATR managed projects are skipped, so if the uploaded CSV contained a file in an unmanaged project that was actually in a managed project, would you not end up with a duplicate? We check that it's not duplicated within the CSV, that the target project isn't managed in ATR, that the committee and release exist, and that the primary key is free, but all of those would pass in this circumstance and yet there'd still be a duplicate.

I've unified the logic between the two paths and made sure we're checking existing managed projects too, not just the snapshot of projects which match those uploaded.

You can move a release in releases, and that moves its artifacts along with it. If you then do an add in artifacts (the separate field for that), using the same name as an artifact's destination in the previous step, the code creates two duplicate rows for the artifact. This then fails because of the unique constraint in the database.

Here's a test case for this one:

def test_classify_collision() -> None:
    snap = _snapshot(
        project_committee={"alpha-one": "alpha", "alpha-two": "alpha"},
        artifact_pks=frozenset({("alpha-one", "1.0.0", "a.tgz")}),
    )
    rows = {
        "releases": [_release("alpha-one-1.0.0", "alpha-two", "1.0.0")],
        "artifacts": [_artifact("alpha-two", "1.0.0", "a.tgz", "alpha/new")],
    }
    d = catalogue_diff.classify_additive(rows, snap, "alpha")
    assert d.counts["conflict"] == 1

(This fails, but it shouldn't.)

Thanks for the test, and good catch! It now tracks the resulting primary keys as it goes along, meaning we should catch duplicates that are both in the uploaded files.

Or paths.get_tmp_dir gives a safe.StatePath.